Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BorrowDatum for unsizing borrows of datums #1891

Merged

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Sep 29, 2024

Add a trait that allows borrowing Datums from various places, like arguments and arrays, instead of always taking them by-value. This is most important for unsizing borrows, as the sized kind are a simple .cast() away.

The motivating rationale is to enable new APIs in the near future like FlatArray and Text. It also brings in some more uniformity because of the blanket-impl of ArgAbi for &T. This means that various types that are pass-by-reference may now actually be taken by reference from their Datum, instead of copying them into the Rust function! This is more important for unsized types, but for various reasons, right now it only takes over the ArgAbi of one unsized type: CStr.

pgrx/src/datum/borrow.rs Outdated Show resolved Hide resolved
pgrx/src/datum/borrow.rs Outdated Show resolved Hide resolved
pgrx/src/callconv.rs Show resolved Hide resolved
pgrx/src/datum/borrow.rs Outdated Show resolved Hide resolved
pgrx/src/datum/borrow.rs Outdated Show resolved Hide resolved
pgrx/src/datum/borrow.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@YohDeadfall YohDeadfall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, cool, I like it even when I'm not familiar with some parts of pgrx. I will take a closer look tomorrow when I come to the keyboard.

pgrx/src/datum/borrow.rs Show resolved Hide resolved
}
}

unsafe fn unbox_nullable_arg(arg: Arg<'_, 'fcx>) -> Nullable<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nullable is a kind of Option, and methods returning an option and their unchecked variants have the same names not counting the suffix. I would move in that direction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't determined yet if I want the base to be a Result type.

pgrx/src/callconv.rs Show resolved Hide resolved
@workingjubilee
Copy link
Member Author

Current main unknown: does this trait need a lifetime in order to provide a correct API...? Or should I just enforce that in interfaces like Datum::borrow_as?

/// are implementing a varlena type. As the other dynamic length type, CStr also does this.
/// This function
/// - must NOT mutate the pointee
/// - must point to the entire datum's length (`size_of_val` must not lose bytes)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be fine if we relax this requirement eventually.

@workingjubilee workingjubilee merged commit 901401c into pgcentralfoundation:develop Oct 1, 2024
14 checks passed
@workingjubilee workingjubilee deleted the add-borrow-datum branch October 1, 2024 21:05
eeeebbbbrrrr added a commit that referenced this pull request Oct 14, 2024
Welcome to pgrx v0.12.6. This releases fixes a number of bugs, cleans up
various parts of the code, adds more Postgres `#include` headers, and
other little things.

To upgrade, first install cargo-pgrx with `cargo install cargo-pgrx
--version 0.12.6 --locked`. Then you can run `cargo pgrx upgrade` in
your extension's crate to update its dependencies.

## What's Changed
* Removed extra allocation from `IntoDatum for char` by @YohDeadfall in
#1887
* Fixed IntoDatum impl for char introduced by #1887 by @YohDeadfall in
#1889
* Support refs to unsized SqlTranslatable types by @workingjubilee in
#1890
* Nullable is Copy when T is Copy by @workingjubilee in
#1884
* Macro based IntoDatum for string types by @YohDeadfall in
#1886
* `impl FromDatum for CString` by @workingjubilee in
#1896
* Add `BorrowDatum` for unsizing borrows of datums by @workingjubilee in
#1891
* Always panic if len of varlena exceeds the maximum by @YohDeadfall in
#1894
* Use cstr literal syntax to avoid unsafe and ??? by @workingjubilee in
#1898
* Implement UnboxDatum for ItemPointerData by @syvb in
#1900
* fix compilation on some CPUs and Windows by @usamoi in
#1901
* docs: update generic type name of TableIterator by @SteveLauC in
#1905
* Remove Python workaround for Homebrew by @workingjubilee in
#1908
* Improved messages for json errors by @YohDeadfall in
#1893
* Adds more headers by @aykut-bozkurt in
#1910
* docs: align description of GucContext::SuBackend and
GucContext::Backend with original PostgreSQL description by @mrl5 in
#1909
* docs: update outdated README by @SteveLauC in
#1914
* fix compilation on emscripten by @usamoi in
#1917
* feat: include 'storage/indexfsm.h' by @SteveLauC in
#1912
* refactor: more CStr literals by @SteveLauC in
#1911
* fix segfault with empty `numrange` during `from_datum()` by
@eeeebbbbrrrr in #1918

## New Contributors
* @mrl5 made their first contribution in
#1909

**Full Changelog**:
v0.12.5...v0.12.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants